feat: Move view cache to Blueprint hierarchy #531
Conversation
…rarchy * origin/main: release: Blueprint 5.1.0 (#528) chore(ios): Scripts for preparing release and changelog (#529) feat: `accessibilityIdentifier` can now be set on `AttributedLabel` (#524) Bump rexml from 3.3.6 to 3.3.9 Prepare 5.0.1 release (#522) Update CHANGELOG.md Bumping versions to 5.0.0. Add precondition to `setNeedsViewHierarchyUpdate`. Expanded precondition message to encourage self-diagnosis. Renamed accessibility(...) to deprecated_accessibility(...) (#516) Annotate `updateViewHierarchyIfNeeded` and `update(node:context:)` with @mainactor and add `preconditionIsolated` runtime check Bump Github jobs to use macos-14 Bump CI Xcode to 15.4 (from 15.1) Add priority to allow adjusting how extra space in a run/row in a flow should be used.
Renamed `elementMeasurer` to `viewCache`. TypeKeyedCache now exposes a `value` function for retrieving the value
| if environment.inheritedViewCache == nil { | ||
| environment.viewCache = viewCache | ||
| } |
There was a problem hiding this comment.
I'm still a little nervous about cascading view caches through the environment. Won't it still leave open the possibility that nested blueprint view elements leak stuff through long(er)-lived caches?
There was a problem hiding this comment.
It's definitely possible, but largely the root blueprint view lives on the screen level.
| static let defaultValue: TypeKeyedCache? = nil | ||
| } | ||
|
|
||
| private static let fallback = TypeKeyedCache() |
There was a problem hiding this comment.
I'm thinking we shouldn't have a static fallback.
There was a problem hiding this comment.
Yeah if we can get away with it. I haven't done an integration in POS yet, and I believe there is older code that just out of band measures elements with an empty environment, so we may need it for now.
| // - Parameter create: A closure that creates a new instance of Value. It | ||
| // is only invoked when a cached value does not exist. The created value is | ||
| // cached for future usage. | ||
| public func value<Value: AnyObject>(_ create: () -> Value) -> Value { |
There was a problem hiding this comment.
FWIW, I went with the "access" pattern with an access closure so people aren't tempted to keep a reference to the cached measurement view, kind of like the swift pointer APIs where the pointer shouldn't be stored outside the access closure.
| @@ -128,16 +128,22 @@ public struct AttributedLabel: Element, Hashable { | |||
| /// You can check if this value should be false via `NSAttributedString.needsNormalizingForView(...)` | |||
| public var needsTextNormalization: Bool = true | |||
|
|
|||
| private static let prototypeLabel = LabelView() | |||
There was a problem hiding this comment.
@watt suggested we consider leaving this one as-is, at least for the initial iteration since its:
- low likelihood of retaining something it shouldn't be (and would be flushed out by other measurements)
- on a relatively hot code path
MeasurementCachetoTypeKeyedCacheBlueprintView.viewMeasurertoviewCache